-
Notifications
You must be signed in to change notification settings - Fork 65
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(component): add autoComplete prop for selects #542
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small comment, but could you actually show how it fixes #540?
@@ -7,6 +7,7 @@ import { SelectOptionGroup } from '../Select/types'; | |||
|
|||
interface BaseSelect extends Omit<React.HTMLAttributes<HTMLInputElement>, 'children'> { | |||
action?: SelectAction; | |||
autoComplete?: string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you check that this is not on React.HTMLAttributes<HTMLInputElement>
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It isn't, I would need to use AllHTMLAttributes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah makes sense. We'd probably want to do this for the Form
component too then. (it's only on | off
for the form
element).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume it defaults to on
on the form?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FormHTMLAttributes
does have autoComplete
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I could swap the type of the Selects for <React.InputHTMLAttributes<HTMLInputElement>
which do include autoComplete
and a bunch of others
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔
Only thing we need to make sure if we do, is that the types we say we allow (for example, valid input attributes like any aria-
ones, or the multiple
attribute) are actually passed in to the underlying component. We don't want our types to say we have something that our component doesn't actually implement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets keep as is since I don't want to expose all those to the user.
8c3e26a
to
6447c66
Compare
6447c66
to
857bcbf
Compare
What
Allow custom values for autoComplete. Defaults to
off
.Why
Fixes #540